Skip to content

🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY#12712

Merged
danny-avila merged 4 commits intofeat/agent-skillsfrom
claude/flamboyant-poincare-49b599
Apr 21, 2026
Merged

🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY#12712
danny-avila merged 4 commits intofeat/agent-skillsfrom
claude/flamboyant-poincare-49b599

Conversation

@danny-avila
Copy link
Copy Markdown
Owner

@danny-avila danny-avila commented Apr 17, 2026

Summary

I decoupled bash_tool registration and skill-file priming from per-user CODE_API_KEY lookups as part of Phase 4 of the Agent Skills umbrella (#12625). LIBRECHAT_CODE_API_KEY is the LibreChat-hosted sandbox service key — a system-level credential, not a per-user secret — so sandbox-touching paths are now gated solely on the execute_code capability, and the key itself is read from process.env inside that gate.

  • Threaded a boolean codeEnvAvailable through enrichWithSkillConfigurable and primeInvokedSkills in place of the old codeApiKey + loadAuthValues plumbing.
  • Gated handleSkillToolCall file priming on codeEnvAvailable from mergedConfigurable so an agent with execute_code disabled cannot trigger sandbox uploads even when the env var is present.
  • Simplified the bash_tool loader in ToolService.js to call createBashExecutionTool({}) — the agents library reads LIBRECHAT_CODE_API_KEY from the environment internally. Removed the "Code execution is not available" placeholder tool that lied to the model in its tool list when the env var was missing.
  • Fixed a pre-existing appConfig-undefined lint error in responses.js/createResponse that surfaced when this file was touched, and finished the extraction so all req.config references in that function now go through appConfig.
  • Hoisted codeEnvAvailable and skillPrimedIdsByName out of the loadTools closures in openai.js and responses.js; both values are fixed once initializeAgent resolves, so recomputing them per tool execution was wasted work.
  • Added packages/api/src/agents/skillConfigurable.spec.ts (5 cases pinning the new surface contract).
  • Added packages/api/src/agents/skillFiles.spec.ts (5 cases covering the 4-way capability × env-key matrix plus an end-to-end upload assertion).
  • Added 3 cases in packages/api/src/agents/handlers.spec.ts covering the codeEnvAvailable gate in handleSkillToolCall.

Out of scope (intentionally untouched): the legacy CodeExecutionTool path in handleTools.js, post-execution file-download callbacks, Programmatic Tool Calling registration, and the per-user execute_code plugin install hook in the frontend (follow-up tracked in #12713).

Change Type

  • Bug fix (non-breaking change which fixes an issue)

Testing

Ran from packages/api:

  • npx jest src/agents/skillConfigurable.spec.ts — 5/5 pass.
  • npx jest src/agents/skillFiles.spec.ts — 5/5 pass.
  • npx jest src/agents/handlers.spec.ts — 20/20 pass (3 new capability-gate cases).
  • npx jest src/agents/__tests__/skills.test.ts — 36/36 pass.
  • npx jest src/agents — 591+ pass; the only failures (2) are a pre-existing ESM dynamic-import issue in summarization.e2e.test.ts, unchanged by this PR.

Ran from api:

  • npx jest server/services/__tests__/ToolService.spec.js — 42/42 pass.

Recommended manual verification in a full install:

  • With LIBRECHAT_CODE_API_KEY set and execute_code enabled on an agent with a skill → skill invocation runs bash commands and primes files.
  • With the env var unset → tool creation fails loudly via the agents library's own error, no crash.
  • With execute_code disabled → bash_tool is absent from definitions and skill file priming is skipped (unchanged).

Test Configuration:

  • Node 20.19.x / npm workspaces
  • Base branch: feat/agent-skills

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Local unit tests pass with my changes

@danny-avila
Copy link
Copy Markdown
Owner Author

@codex review

@danny-avila danny-avila mentioned this pull request Apr 17, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50bb359682

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +457 to 458
const codeApiKey = process.env[EnvVar.CODE_API_KEY] ?? '';
if (codeApiKey) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enforce execute_code capability before priming skill files

This path now primes skill files whenever LIBRECHAT_CODE_API_KEY is present in the server environment, but it no longer checks whether code execution is enabled for the run/agent. In deployments where skills are enabled but execute_code is intentionally disabled, invoking a file-backed skill will still upload files to the code sandbox, causing unintended data egress and background code-env activity. Please gate this branch on the same execute-code capability signal used when registering bash_tool.

Useful? React with 👍 / 👎.

Comment thread packages/api/src/agents/skillFiles.ts Outdated
);
}
}
const apiKey = process.env[EnvVar.CODE_API_KEY] ?? '';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Skip history file re-priming when execute_code is disabled

primeInvokedSkills now derives the API key directly from process env and will re-prime historical skill files whenever that env var is set, even if the current run does not have code execution enabled. That means conversations can trigger sandbox uploads during history reconstruction despite bash_tool being unavailable for the agent, which is both unnecessary and a data-scope regression. Add an execute-code capability gate before entering the priming/upload path.

Useful? React with 👍 / 👎.

danny-avila added a commit that referenced this pull request Apr 17, 2026
Addresses review feedback on #12712: the previous commit gated skill
file priming only on process.env[EnvVar.CODE_API_KEY] presence, which
meant agents with execute_code disabled but the env var present would
still upload bundled skill files to the sandbox on every invocation —
a data-scope regression.

Thread the execute_code capability flag (codeEnvAvailable) through
enrichWithSkillConfigurable → configurable, and as a dep on
primeInvokedSkills. Both handleSkillToolCall and primeInvokedSkills
gate the sandbox-upload path on codeEnvAvailable first; the API key
is then read from env inside that gate (if also missing, skip with
a warn — admin misconfig).

Adds skillFiles.spec.ts pinning the four-way matrix: capability
on/off × env key set/unset.
danny-avila added a commit that referenced this pull request Apr 17, 2026
Addresses review feedback on #12712: the previous commit gated skill
file priming only on process.env[EnvVar.CODE_API_KEY] presence, which
meant agents with execute_code disabled but the env var present would
still upload bundled skill files to the sandbox on every invocation —
a data-scope regression.

Thread the execute_code capability flag (codeEnvAvailable) through
enrichWithSkillConfigurable → configurable, and as a dep on
primeInvokedSkills. Both handleSkillToolCall and primeInvokedSkills
gate the sandbox-upload path on codeEnvAvailable first; the API key
is then read from env inside that gate (if also missing, skip with
a warn — admin misconfig).

Adds skillFiles.spec.ts pinning the four-way matrix: capability
on/off × env key set/unset.
@danny-avila danny-avila force-pushed the claude/flamboyant-poincare-49b599 branch from be95291 to c7c73a2 Compare April 17, 2026 16:32
@danny-avila danny-avila marked this pull request as draft April 19, 2026 15:40
@danny-avila danny-avila force-pushed the claude/flamboyant-poincare-49b599 branch from c7c73a2 to f30038b Compare April 21, 2026 15:50
@danny-avila
Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danny-avila
Copy link
Copy Markdown
Owner Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f30038bcc1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 1273 to +1277
const isBashTool = toolNames.includes(AgentConstants.BASH_TOOL);
if (isBashTool) {
try {
const authValues = await loadAuthValues({
userId: req.user.id,
authFields: [EnvVar.CODE_API_KEY],
});
const codeApiKey = authValues[EnvVar.CODE_API_KEY];

if (codeApiKey) {
const bashTool = createBashExecutionTool({ apiKey: codeApiKey });
allLoadedTools.push(bashTool);
} else {
logger.debug('[loadToolsForExecution] bash_tool requested but CODE_API_KEY not available');
allLoadedTools.push(
toolFn(
async () => [
'Code execution is not available. Use the read_file tool instead.',
undefined,
],
{
name: AgentConstants.BASH_TOOL,
description: 'Bash execution (unavailable - no code API key configured)',
schema: { type: 'object', properties: { command: { type: 'string' } } },
responseFormat: AgentConstants.CONTENT_AND_ARTIFACT,
},
),
);
}
const bashTool = createBashExecutionTool({});
allLoadedTools.push(bashTool);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Gate bash_tool loading on execute_code capability

This branch now creates bash_tool solely from the requested tool name and no longer enforces the execute_code capability before loading it. Because ON_TOOL_EXECUTE loads tools from model-emitted tool-call names, a stray/hallucinated bash_tool call can still be executed when execute_code is disabled for that agent/tenant if LIBRECHAT_CODE_API_KEY exists in server env. Add an explicit capability/allowlist check before constructing and registering bash_tool.

Useful? React with 👍 / 👎.

danny-avila added a commit that referenced this pull request Apr 21, 2026
Resolves findings from the second codex review on #12712:

- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
  `handleSkillToolCall` across three cases (gate off, gate on + env
  set, gate on + env unset). The gate is the critical regression
  prevention — a future edit that drops it would silently re-enable
  sandbox uploads for agents with `execute_code` disabled.

- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
  `loadTools` closures in `openai.js` and `responses.js`. Both values
  are fixed once `initializeAgent` resolves, so recomputing them on
  every tool execution was wasted work. `responses.js` shares a single
  pair between its streaming and non-streaming branches.

- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
  upload path end-to-end with real file records, asserting
  `batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
  the correct file set (including the synthetic `SKILL.md`).

- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
  — replaces the remaining `req.config` references with `appConfig` for
  consistency with the pattern in other controllers.

No behavioral changes beyond what was already in place; this is
coverage and readability polish.
@danny-avila
Copy link
Copy Markdown
Owner Author

Audited the review — disposition:

Fixed in a16c612:

  • Finding 1 (MAJOR): Added a new describe('skill tool codeEnvAvailable gate …') block in handlers.spec.ts with 3 cases: gate off + env set, gate on + env set, gate on + env unset. All assert listSkillFiles is (or isn't) called.
  • Finding 2 (MINOR): Hoisted codeEnvAvailable and skillPrimedIdsByName out of the loadTools closures in openai.js and responses.js. responses.js shares a single pair across its streaming and non-streaming branches.
  • Finding 4 (MINOR): Added a new skillFiles.spec.ts case that mocks real file records and asserts batchUploadCodeEnvFiles is called with the env-sourced apiKey and the correct file set (including the synthetic SKILL.md).
  • Finding 5 (NIT): Finished the appConfig extraction in createResponse — replaced all remaining req.config references.
  • Finding 6 (NIT): Updated the PR description with accurate test counts and scope notes.

Rejected with rationale:

  • Finding 3 (MINOR): The placeholder tool was intentionally removed. It lied to the model in its tool list (description: "Bash execution (unavailable - no code API key configured)") and then returned a canned "use read_file instead" message at call time, which could cause the model to loop or hallucinate. The replacement behavior — capability upstream gates whether bash_tool is ever in the catalog at all; if it somehow reaches loadToolsForExecution without an env key, the library's own error is logged with the exact env-var name for ops — is cleaner. This aligns with the goal stated in the PR: LIBRECHAT_CODE_API_KEY is no longer a requirement for the feature, just an admin-level config.
  • Finding 7 (NIT): .spec.ts is the convention at the top level of packages/api/src/agents/ (all 11 existing files: handlers.spec.ts, avatars.spec.ts, config.spec.ts, etc.). .test.ts is the convention inside the __tests__/ subdirectory. My new files match their location.

CI still green.

@danny-avila
Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danny-avila
Copy link
Copy Markdown
Owner Author

Follow-up review audited — both NITs addressed in 6e69643:

  • NIT 1 (as unknown as never cast on _id): Only fixed the one I introduced in makeSkillHandlerWithFiles. Replaced with a real new Types.ObjectId() (via jest.requireActual('mongoose'), matching the primed-skill test helpers earlier in the same file) and updated the matching toHaveBeenCalledWith to pass the ObjectId directly. Left the other pre-existing as unknown as never sites alone — they're outside this PR's scope.

  • NIT 2 (fragile stream mock): Swapped { on, pipe, read } stub for Readable.from(Buffer.from('')) in the upload-path test. Added a comment noting the real Readable matches the production contract and is robust to any future partial-real replacement of batchUploadCodeEnvFiles. Added import { Readable } from 'stream'.

On the flagged CI api failure: ran npx jest server/controllers/agents/__tests__/responses.unit.spec.js locally against a16c6126e (the commit CI failed on) — 11/11 pass. The CI log shows "worker failed to exit gracefully" plus 1284 MB heap usage on that file, consistent with test leakage from another file in the same suite run rather than a real assertion failure on my change. This push will re-run CI to confirm.

Phase 4 of Agent Skills umbrella (#12625): gate bash_tool and skill
file priming on the `execute_code` capability only. Thread a boolean
`codeEnvAvailable` through `enrichWithSkillConfigurable` and
`primeInvokedSkills` in place of the old per-user `codeApiKey` +
`loadAuthValues` plumbing. The sandbox API key is the LibreChat-
hosted service key — system-level, not a user secret — so the
per-user lookup was legacy; when needed, it's read directly from
`process.env[EnvVar.CODE_API_KEY]` inside the capability gate.

`handleSkillToolCall` and `primeInvokedSkills` gate sandbox uploads
on `codeEnvAvailable` first, preventing skill-file uploads to the
sandbox when an agent has `execute_code` disabled even if the env
var happens to be set. The agents library resolves the env key
itself for `bash_tool`, so `ToolService.js` drops the
`loadAuthValues` lookup and the "Code execution is not available"
placeholder tool in favor of a plain `createBashExecutionTool({})`
with a loud error log if the env var is missing.

Also fixes a pre-existing `appConfig`-undefined lint error in
`responses.js`/`createResponse` that surfaced when this file was
touched (declares `const appConfig = req.config` at function top,
matching the existing pattern in other controllers).

Preserves the `skillPrimedIdsByName` threading added by Phase 3/5/6
and all Phase 3/5/6 call-site signatures. Adds
`skillConfigurable.spec.ts` (5 cases pinning the new surface) and
`skillFiles.spec.ts` (4-way matrix of capability × env key for
`primeInvokedSkills`).
Resolves findings from the second codex review on #12712:

- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
  `handleSkillToolCall` across three cases (gate off, gate on + env
  set, gate on + env unset). The gate is the critical regression
  prevention — a future edit that drops it would silently re-enable
  sandbox uploads for agents with `execute_code` disabled.

- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
  `loadTools` closures in `openai.js` and `responses.js`. Both values
  are fixed once `initializeAgent` resolves, so recomputing them on
  every tool execution was wasted work. `responses.js` shares a single
  pair between its streaming and non-streaming branches.

- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
  upload path end-to-end with real file records, asserting
  `batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
  the correct file set (including the synthetic `SKILL.md`).

- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
  — replaces the remaining `req.config` references with `appConfig` for
  consistency with the pattern in other controllers.

No behavioral changes beyond what was already in place; this is
coverage and readability polish.
Round-3 codex review flagged two NITs on the test code added in the
previous commit:

- Replace `_id: 'skill-id' as unknown as never` in the new
  `makeSkillHandlerWithFiles` helper with a real `Types.ObjectId`,
  matching the pattern used by the primed-skill tests further up in
  the same file (and by `skillFiles.spec.ts`). The `never` cast
  hides the fact that `_id` really is a string / ObjectId at runtime.

- Replace the ad-hoc `{ on, pipe, read }` stub with a real
  `Readable.from(Buffer.from(''))` in the upload-path test. The stub
  worked only because `batchUploadCodeEnvFiles` is mocked and never
  iterates the stream; `Readable.from` satisfies the same contract
  and is robust to any future partial-real replacement of the upload
  function.

Pure test-hygiene improvements; no runtime code touched.
The upstream `2463b6acd` fix declared `const appConfig = req.config`
inside the try block (line 381) to patch the same `no-undef` error
I fixed in this PR at the top of `createResponse` (line 283). The
rebase kept both declarations side-by-side. Drops the inner one —
the outer binding covers every downstream reference already.
@danny-avila danny-avila force-pushed the claude/flamboyant-poincare-49b599 branch from 6e69643 to 66037f3 Compare April 21, 2026 17:57
@danny-avila danny-avila marked this pull request as ready for review April 21, 2026 18:04
@danny-avila danny-avila merged commit d31fa1b into feat/agent-skills Apr 21, 2026
9 checks passed
@danny-avila danny-avila deleted the claude/flamboyant-poincare-49b599 branch April 21, 2026 18:04
danny-avila added a commit that referenced this pull request Apr 22, 2026
* 🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY

Phase 4 of Agent Skills umbrella (#12625): gate bash_tool and skill
file priming on the `execute_code` capability only. Thread a boolean
`codeEnvAvailable` through `enrichWithSkillConfigurable` and
`primeInvokedSkills` in place of the old per-user `codeApiKey` +
`loadAuthValues` plumbing. The sandbox API key is the LibreChat-
hosted service key — system-level, not a user secret — so the
per-user lookup was legacy; when needed, it's read directly from
`process.env[EnvVar.CODE_API_KEY]` inside the capability gate.

`handleSkillToolCall` and `primeInvokedSkills` gate sandbox uploads
on `codeEnvAvailable` first, preventing skill-file uploads to the
sandbox when an agent has `execute_code` disabled even if the env
var happens to be set. The agents library resolves the env key
itself for `bash_tool`, so `ToolService.js` drops the
`loadAuthValues` lookup and the "Code execution is not available"
placeholder tool in favor of a plain `createBashExecutionTool({})`
with a loud error log if the env var is missing.

Also fixes a pre-existing `appConfig`-undefined lint error in
`responses.js`/`createResponse` that surfaced when this file was
touched (declares `const appConfig = req.config` at function top,
matching the existing pattern in other controllers).

Preserves the `skillPrimedIdsByName` threading added by Phase 3/5/6
and all Phase 3/5/6 call-site signatures. Adds
`skillConfigurable.spec.ts` (5 cases pinning the new surface) and
`skillFiles.spec.ts` (4-way matrix of capability × env key for
`primeInvokedSkills`).

* 🧪 refactor: Address Codex Review Feedback

Resolves findings from the second codex review on #12712:

- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
  `handleSkillToolCall` across three cases (gate off, gate on + env
  set, gate on + env unset). The gate is the critical regression
  prevention — a future edit that drops it would silently re-enable
  sandbox uploads for agents with `execute_code` disabled.

- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
  `loadTools` closures in `openai.js` and `responses.js`. Both values
  are fixed once `initializeAgent` resolves, so recomputing them on
  every tool execution was wasted work. `responses.js` shares a single
  pair between its streaming and non-streaming branches.

- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
  upload path end-to-end with real file records, asserting
  `batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
  the correct file set (including the synthetic `SKILL.md`).

- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
  — replaces the remaining `req.config` references with `appConfig` for
  consistency with the pattern in other controllers.

No behavioral changes beyond what was already in place; this is
coverage and readability polish.

* 🧷 test: Tighten Spec Hygiene Per Codex Nit Feedback

Round-3 codex review flagged two NITs on the test code added in the
previous commit:

- Replace `_id: 'skill-id' as unknown as never` in the new
  `makeSkillHandlerWithFiles` helper with a real `Types.ObjectId`,
  matching the pattern used by the primed-skill tests further up in
  the same file (and by `skillFiles.spec.ts`). The `never` cast
  hides the fact that `_id` really is a string / ObjectId at runtime.

- Replace the ad-hoc `{ on, pipe, read }` stub with a real
  `Readable.from(Buffer.from(''))` in the upload-path test. The stub
  worked only because `batchUploadCodeEnvFiles` is mocked and never
  iterates the stream; `Readable.from` satisfies the same contract
  and is robust to any future partial-real replacement of the upload
  function.

Pure test-hygiene improvements; no runtime code touched.

* 🧹 chore: Remove Duplicate appConfig Declaration After Rebase

The upstream `2463b6acd` fix declared `const appConfig = req.config`
inside the try block (line 381) to patch the same `no-undef` error
I fixed in this PR at the top of `createResponse` (line 283). The
rebase kept both declarations side-by-side. Drops the inner one —
the outer binding covers every downstream reference already.
danny-avila added a commit that referenced this pull request Apr 22, 2026
* 🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY

Phase 4 of Agent Skills umbrella (#12625): gate bash_tool and skill
file priming on the `execute_code` capability only. Thread a boolean
`codeEnvAvailable` through `enrichWithSkillConfigurable` and
`primeInvokedSkills` in place of the old per-user `codeApiKey` +
`loadAuthValues` plumbing. The sandbox API key is the LibreChat-
hosted service key — system-level, not a user secret — so the
per-user lookup was legacy; when needed, it's read directly from
`process.env[EnvVar.CODE_API_KEY]` inside the capability gate.

`handleSkillToolCall` and `primeInvokedSkills` gate sandbox uploads
on `codeEnvAvailable` first, preventing skill-file uploads to the
sandbox when an agent has `execute_code` disabled even if the env
var happens to be set. The agents library resolves the env key
itself for `bash_tool`, so `ToolService.js` drops the
`loadAuthValues` lookup and the "Code execution is not available"
placeholder tool in favor of a plain `createBashExecutionTool({})`
with a loud error log if the env var is missing.

Also fixes a pre-existing `appConfig`-undefined lint error in
`responses.js`/`createResponse` that surfaced when this file was
touched (declares `const appConfig = req.config` at function top,
matching the existing pattern in other controllers).

Preserves the `skillPrimedIdsByName` threading added by Phase 3/5/6
and all Phase 3/5/6 call-site signatures. Adds
`skillConfigurable.spec.ts` (5 cases pinning the new surface) and
`skillFiles.spec.ts` (4-way matrix of capability × env key for
`primeInvokedSkills`).

* 🧪 refactor: Address Codex Review Feedback

Resolves findings from the second codex review on #12712:

- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
  `handleSkillToolCall` across three cases (gate off, gate on + env
  set, gate on + env unset). The gate is the critical regression
  prevention — a future edit that drops it would silently re-enable
  sandbox uploads for agents with `execute_code` disabled.

- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
  `loadTools` closures in `openai.js` and `responses.js`. Both values
  are fixed once `initializeAgent` resolves, so recomputing them on
  every tool execution was wasted work. `responses.js` shares a single
  pair between its streaming and non-streaming branches.

- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
  upload path end-to-end with real file records, asserting
  `batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
  the correct file set (including the synthetic `SKILL.md`).

- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
  — replaces the remaining `req.config` references with `appConfig` for
  consistency with the pattern in other controllers.

No behavioral changes beyond what was already in place; this is
coverage and readability polish.

* 🧷 test: Tighten Spec Hygiene Per Codex Nit Feedback

Round-3 codex review flagged two NITs on the test code added in the
previous commit:

- Replace `_id: 'skill-id' as unknown as never` in the new
  `makeSkillHandlerWithFiles` helper with a real `Types.ObjectId`,
  matching the pattern used by the primed-skill tests further up in
  the same file (and by `skillFiles.spec.ts`). The `never` cast
  hides the fact that `_id` really is a string / ObjectId at runtime.

- Replace the ad-hoc `{ on, pipe, read }` stub with a real
  `Readable.from(Buffer.from(''))` in the upload-path test. The stub
  worked only because `batchUploadCodeEnvFiles` is mocked and never
  iterates the stream; `Readable.from` satisfies the same contract
  and is robust to any future partial-real replacement of the upload
  function.

Pure test-hygiene improvements; no runtime code touched.

* 🧹 chore: Remove Duplicate appConfig Declaration After Rebase

The upstream `2463b6acd` fix declared `const appConfig = req.config`
inside the try block (line 381) to patch the same `no-undef` error
I fixed in this PR at the top of `createResponse` (line 283). The
rebase kept both declarations side-by-side. Drops the inner one —
the outer binding covers every downstream reference already.
danny-avila added a commit that referenced this pull request Apr 25, 2026
* 🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY

Phase 4 of Agent Skills umbrella (#12625): gate bash_tool and skill
file priming on the `execute_code` capability only. Thread a boolean
`codeEnvAvailable` through `enrichWithSkillConfigurable` and
`primeInvokedSkills` in place of the old per-user `codeApiKey` +
`loadAuthValues` plumbing. The sandbox API key is the LibreChat-
hosted service key — system-level, not a user secret — so the
per-user lookup was legacy; when needed, it's read directly from
`process.env[EnvVar.CODE_API_KEY]` inside the capability gate.

`handleSkillToolCall` and `primeInvokedSkills` gate sandbox uploads
on `codeEnvAvailable` first, preventing skill-file uploads to the
sandbox when an agent has `execute_code` disabled even if the env
var happens to be set. The agents library resolves the env key
itself for `bash_tool`, so `ToolService.js` drops the
`loadAuthValues` lookup and the "Code execution is not available"
placeholder tool in favor of a plain `createBashExecutionTool({})`
with a loud error log if the env var is missing.

Also fixes a pre-existing `appConfig`-undefined lint error in
`responses.js`/`createResponse` that surfaced when this file was
touched (declares `const appConfig = req.config` at function top,
matching the existing pattern in other controllers).

Preserves the `skillPrimedIdsByName` threading added by Phase 3/5/6
and all Phase 3/5/6 call-site signatures. Adds
`skillConfigurable.spec.ts` (5 cases pinning the new surface) and
`skillFiles.spec.ts` (4-way matrix of capability × env key for
`primeInvokedSkills`).

* 🧪 refactor: Address Codex Review Feedback

Resolves findings from the second codex review on #12712:

- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
  `handleSkillToolCall` across three cases (gate off, gate on + env
  set, gate on + env unset). The gate is the critical regression
  prevention — a future edit that drops it would silently re-enable
  sandbox uploads for agents with `execute_code` disabled.

- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
  `loadTools` closures in `openai.js` and `responses.js`. Both values
  are fixed once `initializeAgent` resolves, so recomputing them on
  every tool execution was wasted work. `responses.js` shares a single
  pair between its streaming and non-streaming branches.

- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
  upload path end-to-end with real file records, asserting
  `batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
  the correct file set (including the synthetic `SKILL.md`).

- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
  — replaces the remaining `req.config` references with `appConfig` for
  consistency with the pattern in other controllers.

No behavioral changes beyond what was already in place; this is
coverage and readability polish.

* 🧷 test: Tighten Spec Hygiene Per Codex Nit Feedback

Round-3 codex review flagged two NITs on the test code added in the
previous commit:

- Replace `_id: 'skill-id' as unknown as never` in the new
  `makeSkillHandlerWithFiles` helper with a real `Types.ObjectId`,
  matching the pattern used by the primed-skill tests further up in
  the same file (and by `skillFiles.spec.ts`). The `never` cast
  hides the fact that `_id` really is a string / ObjectId at runtime.

- Replace the ad-hoc `{ on, pipe, read }` stub with a real
  `Readable.from(Buffer.from(''))` in the upload-path test. The stub
  worked only because `batchUploadCodeEnvFiles` is mocked and never
  iterates the stream; `Readable.from` satisfies the same contract
  and is robust to any future partial-real replacement of the upload
  function.

Pure test-hygiene improvements; no runtime code touched.

* 🧹 chore: Remove Duplicate appConfig Declaration After Rebase

The upstream `2463b6acd` fix declared `const appConfig = req.config`
inside the try block (line 381) to patch the same `no-undef` error
I fixed in this PR at the top of `createResponse` (line 283). The
rebase kept both declarations side-by-side. Drops the inner one —
the outer binding covers every downstream reference already.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant